-
Notifications
You must be signed in to change notification settings - Fork 179
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added tracing using opentelemetry for jms like kafka does. #2727
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this! Looks good overall. I've noted some small changes that are required.
We can also rename variables kafkaTrace
to jmsTrace
.
There are some disabled tests. Do you need any help with those?
smallrye-reactive-messaging-jms/src/main/java/io/smallrye/reactive/messaging/jms/JmsSink.java
Outdated
Show resolved
Hide resolved
...ing-jms/src/main/java/io/smallrye/reactive/messaging/jms/tracing/JmsAttributesExtractor.java
Outdated
Show resolved
Hide resolved
@dankristensen I've fixed the tests and removed completely client id and group id from the JmsTrace, I didn't know how to fill them. |
@ozangunalp That is great. I have time to look at this tommorow, I will try to get it running locally, and see that i actually get these telemetries shown correctly in jaeger. |
Perfect, thanks for letting me know! |
@dankristensen did you have any time to check this ? |
@ozangunalp Sorry for not getting back to you. I got serious sick, but is back to work again now. I will try to build one, so i can verify if it works correctly |
I have tried, and can see, that i MUST have this dependency available: smallrye-reactive-messaging-otel. This is because it is used in JmsOpenTelemetryInstrumenter. Is this as wanted? Apparently it looks like kafka has same issue. I can see from the documentation, that you must include this. But the error is a little confusing, because tracing is per default enabled, and therefore i get this error. What do you think? |
I can confirm, that i get traces into jaeger now. But i am a little in doubt about to to pass span's across jms messages. Do you have any suggestions for this? |
1e7221e
to
0459ff4
Compare
@dankristensen indeed for trace propagation, we needed to make sure that trace information (traceparent) was set on the outgoing message properties. It was missing. I've fixed it. I think this is good to go. Thanks for your contribution! |
@ozangunalp i have just built a snapshot and tried it. It helped with the traceparent, but it is not ALL of them that is being propagated correctly. I now see a rest POST call, that is publishing a message, and then receiving the message again, in the same span. When recieving the message again, it is doing some operation, and then publishing a new message, which is not aggregated in the same span. Should that not be the case, with the change you made to traceparent, or do i need to adjust something in my app for this? Furthermore i can see, that it would be good, if we could get the channel name as part of the tags we publish. I have looked a bit into this, but i cannot see an obvious place to handle this. Do you have any suggestions for this? |
So the app does the following ?
I don't think we do this for other connectors supporting tracing, if you can come up with a proposition that would be awesome. |
That is correct @ozangunalp . receive POST -> publish message -> JMS -> consume message -> publish message -> JMS And the publish/consume is actually done multiple times in the same microservice, before leaving to another microservice. So i guess this should be in the same span, as the original receive POST. Do you not agree in this? |
@dankristensen Can you push your test to a repository so that I can take a look? |
So you mean create a test in smallrye-reactive-messaging-jms module? Currently i only see this in our working code locally. But i will try to make a small reproducer application, so it is easier for you to troubleshoot. Regarding the channel, i will try to discuss this with one of my colleagues, when he is back from vacation. Maybe we can find a solution for this together |
@ozangunalp Here is a reproducer: https://github.com/dankristensen/srmj-trace-reproducer. |
@dankristensen Thanks for the reproducer. But it changes completely how messages are dispatched: Note that changing that may break some assumptions from app developers, but context propagation and different execution modes (event-loop, blocking, VT). will work better on Quarkus. |
I can see, that you have pushed changes @ozangunalp. Do i need to try it again in a real world example? |
@dankristensen if you do that'll be plus yes! |
@dankristensen I am in favor of getting this in. wdyt ? |
Give me an hour, i am currently testing in a real world scenario. Will get back to you @ozangunalp |
I see a problem, that i cannot fix. In JmsOpenTelemetryInstrumenter line 38, we use MessageOperation, that is from an incubator package. Is it possible, to avoid using that one in any way? |
I currently have a problem in my service, and do not get the multi propagation executed correctly. It could be a mistake on my end, since we have also done some migration to github of the service i am working on. I will get back to you tommorow, where i have found a solution for this. Hopefully that is ok @ozangunalp ? |
Thanks for looking to this. I am thinking that this is already fine for this iteration. We can merge this and iron out the edges on the next. |
I can confirm, that the trace is now working across multiple publish / receive events. But i had to change something in my service, to get it working. In particular i could not use MutinyEmitter.sendAndAwait, but had to use MutinyEmitter.sendAndForget instead. I am using this in application.properties: quarkus.messaging.blocking.signatures.execution.mode=event-loop |
That is expected, before JMS messages were dispatched on the poller worker thread pool. Now they are dispatched on the Vert.x context, whether in blocking worker pool or the Vert.x event-loop. |
This is great @ozangunalp . Thanks alot for your cooperation with this PR. That is fantastic. Looking forward to the release. |
JMS Connector did not have tracing capabilities like kafka did.
Here is a PR, that contains this functionality.
Please review and give me feedback on this